Skip to content

support path-scoped registry credentials#1109

Merged
tdewanNvidia merged 5 commits into
mainfrom
tdewan/path-scoped-registry-credentials
Jun 30, 2026
Merged

support path-scoped registry credentials#1109
tdewanNvidia merged 5 commits into
mainfrom
tdewan/path-scoped-registry-credentials

Conversation

@tdewanNvidia

@tdewanNvidia tdewanNvidia commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Description

OSMO previously resolved Docker registry credentials using only the registry hostname. This prevented users from configuring separate credentials for different repository paths under the same registry host.

Issue #1113

This PR adds path-scoped registry credential support and aligns OSMO more closely with Kubernetes docker config matching behavior.

What Changed

  • Preserve path-scoped registry profiles when credentials are created.
  • Add shared registry-scope matching helpers.
    • Match by path segment, not raw string prefix.
    • Prefer more specific scopes first.
    • Include non-default registry ports in scope matching.
    • Canonicalize the default HTTPS port so equivalent scopes match.
  • Update workflow image validation to try all matching registry credentials.
  • Update generated Kubernetes dockerconfigjson image pull secrets to include all matching auth entries.
  • Update CLI/UI text to clarify that registry credentials may target either a registry host or a registry path.
  • Normalize bulk registry credential lookup keys and decrypt only credentials that match the image scope.

Matching Behavior

For an image like:

<registry-host>/team-b/service/client

These credential scopes match:

  • <registry-host>/team-b
  • <registry-host>

These credential scopes do not match:

  • <registry-host>/team-a
  • <registry-host>/team-b/subpath

The generated docker config includes all matching credentials, ordered most-specific first.

Testing

Bazel Tests

bazel test \
  //src/lib/utils/tests:test_common \
  //src/utils/job/tests:test_task_pure \
  //src/utils/job/tests:test_task \
  //src/utils/job/tests:test_workflow_helpers \
  //src/service/core/workflow/tests:test_helpers \
  //src/cli/tests:test_credential

Result:

Executed 1 out of 6 tests: 6 tests pass.
INFO: Build completed successfully

Manual CLI Validation

Using the Bazel-built CLI, created multiple registry credentials under the same host:

  • one host-level credential
  • one parent path credential
  • one nested child path credential
  • one unrelated sibling path credential

Then verified:

  • credential list output preserved each distinct registry profile
  • an image under the parent path matched the parent path credential and host-level credential
  • the unrelated sibling path credential did not match
  • the nested child path credential did not match images outside that child path

This validates that OSMO now stores path-scoped registry credentials separately and resolves only path-segment-compatible credentials for image validation and image pull secret generation.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

Summary by CodeRabbit

  • New Features
    • Registry credential selection is now path-scoped (host[/repository/path]), improving accuracy for image pulls and registry validation.
    • Registry authentication now attempts all applicable matching scopes (most-specific first) until one succeeds.
  • Bug Fixes
    • Disabled-registry validation now honors path-scoped disable rules and matching more robustly (including normalized scopes and port/path handling).
  • Documentation
    • Updated osmo credential set CLI help example for REGISTRY.
    • Added a design document describing registry-scope normalization and matching.
  • Tests
    • Expanded unit tests for scope normalization, matching order, and credential lookup/validation behavior.

@tdewanNvidia tdewanNvidia requested a review from a team as a code owner June 16, 2026 02:01
@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 58f5cedc-2f23-4bfb-ad54-49fa531d63e5

📥 Commits

Reviewing files that changed from the base of the PR and between 7673077 and 3fd697e.

📒 Files selected for processing (1)
  • src/utils/job/task.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/utils/job/task.py

📝 Walkthrough

Walkthrough

Registry credential lookup, validation, and task credential generation now use normalized registry scopes and path-aware matching. The PostgreSQL connector adds bulk and filtered registry credential retrieval. CLI help text for REGISTRY credentials is updated.

Changes

Path-scoped registry credential matching

Layer / File(s) Summary
Registry scope utility functions
src/lib/utils/common.py, src/lib/utils/tests/test_common.py, projects/path-scoped-registry-credentials.md
Adds registry-scope normalization and matching helpers in common.py; tests cover normalization, path-segment containment, specificity ordering, and port-aware matching; the design document describes canonical scope handling and matching behavior.
PostgreSQL bulk and matching credential retrieval
src/utils/connectors/postgres.py, src/utils/job/tests/test_task_pure.py
Normalizes registry scope in get_registry_cred; adds bulk registry credential retrieval and image-aware matching retrieval in the PostgreSQL connector; tests cover scope normalization and matching behavior.
UserRegistryCredential scope-based validation
src/service/core/workflow/objects.py, src/service/core/workflow/tests/test_helpers.py
valid_cred() now normalizes registry scopes, skips validation for disabled parent scopes, and derives the registry host from the normalized scope; the new test verifies the registry auth call.
WorkflowSpec.validate_registry scope-based auth
src/utils/job/workflow.py, src/utils/job/tests/test_workflow_helpers.py
validate_registry now disables validation by scope match, iterates matching registry credentials for authentication, and uses image_scope in error context; tests cover matching path-scoped credentials and disabled parent scopes.
TaskGroup._get_registry_creds refactor
src/utils/job/task.py, src/utils/job/tests/test_task.py
TaskGroup._get_registry_creds now fetches all user registry credentials once, matches them per image scope, stores normalized scope keys, and uses docker_auth() for auth payloads; tests cover scope matching and normalized OSMO registry credentials.
REGISTRY credential CLI example
src/cli/credential.py
Changes the osmo credential set help example for REGISTRY credentials to use registry=your_registry_or_path.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I hop through scopes from path to path,
No host-only guess can stop my math.
The right credential now finds its way,
With tidy keys to light the day.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.21% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding support for path-scoped registry credentials.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch tdewan/path-scoped-registry-credentials

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 69.76744% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.65%. Comparing base (022c533) to head (3fd697e).
⚠️ Report is 37 commits behind head on main.

Files with missing lines Patch % Lines
src/utils/job/task.py 25.00% 5 Missing and 1 partial ⚠️
src/utils/connectors/postgres.py 20.00% 4 Missing ⚠️
src/utils/job/workflow.py 33.33% 1 Missing and 1 partial ⚠️
src/service/core/workflow/objects.py 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1109      +/-   ##
==========================================
+ Coverage   57.41%   60.65%   +3.23%     
==========================================
  Files         211      201      -10     
  Lines       26658    25562    -1096     
  Branches     4046     3836     -210     
==========================================
+ Hits        15306    15504     +198     
+ Misses      10659     9372    -1287     
+ Partials      693      686       -7     
Flag Coverage Δ
backend 62.96% <69.76%> (+3.32%) ⬆️
ui 35.19% <ø> (+0.46%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/cli/credential.py 100.00% <ø> (ø)
src/lib/utils/common.py 93.69% <100.00%> (-0.65%) ⬇️
src/service/core/workflow/objects.py 58.55% <75.00%> (+0.44%) ⬆️
src/utils/job/workflow.py 60.58% <33.33%> (+0.87%) ⬆️
src/utils/connectors/postgres.py 71.83% <20.00%> (+0.66%) ⬆️
src/utils/job/task.py 63.76% <25.00%> (-0.08%) ⬇️

... and 39 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/utils/common.py`:
- Around line 459-465: The normalize_registry_scope() function needs to
canonicalize the default HTTPS port to match the behavior of
image_registry_scope(). After calling registry_parse() on the parsed netloc to
get the registry value, strip the explicit default port 443 (`:443`) from the
registry string before using it in the return statement. This ensures that
equivalent scopes like `registry.example.com:443/org` and
`registry.example.com/org` normalize to the same value and can be properly
matched.

In `@src/utils/connectors/postgres.py`:
- Around line 1562-1566: The bulk registry credential API returns raw
row.profile keys without normalization, causing inconsistency with the
single-item get_registry_cred() function which normalizes lookup keys. In the
dictionary comprehension that returns the registry credentials (in the section
showing row.profile as the key), normalize the row.profile value using the same
normalization logic applied in get_registry_cred() before using it as the
dictionary key. This ensures equivalent scopes behave consistently between
direct and bulk lookups and properly matches path-scoped credentials regardless
of whether persisted profiles are already in canonical form.
- Around line 1574-1579: Refactor this code to filter matching registry scopes
before decrypting credentials rather than decrypting all and filtering. Instead
of calling get_all_registry_creds(user) which decrypts every credential, first
get only the registry credential keys, use common.matching_registry_scopes() to
identify which scopes match the image_info, and then decrypt only those specific
matching credentials. This prevents unrelated bad credentials from failing valid
image auth paths and eliminates unnecessary decryption work.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f835dd78-05d8-4c80-a9d4-e29a5289da8e

📥 Commits

Reviewing files that changed from the base of the PR and between 022c533 and 4b1699a.

📒 Files selected for processing (10)
  • src/cli/credential.py
  • src/lib/utils/common.py
  • src/lib/utils/tests/test_common.py
  • src/service/core/workflow/objects.py
  • src/service/core/workflow/tests/test_helpers.py
  • src/utils/connectors/postgres.py
  • src/utils/job/task.py
  • src/utils/job/tests/test_task.py
  • src/utils/job/tests/test_workflow_helpers.py
  • src/utils/job/workflow.py

Comment thread src/lib/utils/common.py
Comment thread src/utils/connectors/postgres.py Outdated
Comment thread src/utils/connectors/postgres.py Outdated
Comment thread src/utils/job/task.py Outdated
Comment thread src/utils/job/task.py Outdated
@github-actions

Copy link
Copy Markdown

RyaliNvidia
RyaliNvidia previously approved these changes Jun 30, 2026
@tdewanNvidia tdewanNvidia enabled auto-merge (squash) June 30, 2026 21:02

@xutongNV xutongNV left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check if UI needs to be modified

@tdewanNvidia tdewanNvidia merged commit 109d0cd into main Jun 30, 2026
14 of 18 checks passed
@tdewanNvidia tdewanNvidia deleted the tdewan/path-scoped-registry-credentials branch June 30, 2026 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants